W-23692110: Multiple isolated DataWeave engines per process (Node) - #157
W-23692110: Multiple isolated DataWeave engines per process (Node)#157mlischetti wants to merge 136 commits into
Conversation
|
Pushed remediation for the two code reviews (
A final whole-branch review across all 14 commits came back clean (no Critical/Important findings); the two Minor findings it raised (native-level test for the "Unknown engine handle" JSON contract, and stray review-notes files) are fixed in the last two commits. |
Root-cause fix for the three findings in the sixth PR #157 follow-up review: model the DataWeave instance lifecycle explicitly (uninitialized/ready/ cleaning-up) instead of a single boolean, make C-side stream/transform admission atomic under g_mutex, and validate napi_get_value_int64 at the handle-read sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root-cause fix for the three findings in the sixth PR #157 follow-up review: model the DataWeave instance lifecycle explicitly (uninitialized/ready/ cleaning-up) instead of a single boolean, make C-side stream/transform admission atomic under g_mutex, and validate napi_get_value_int64 at the handle-read sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2a038f2 to
d504c0f
Compare
Round 11 hardening pushed (
|
| # | Finding | Commit |
|---|---|---|
| — | Extract bridge_begin_op_locked (atomic admission-time pin) |
7f24fc7 |
| #2 | Pin engine at admission for streaming + transform (in_flight++ in the same locked section as g_active_ops++; released exactly once on every early-return) |
27cc714 |
| #3 | Pin engine for the synchronous runScriptEngine |
4950380 |
| #1 | Register an env cleanup hook for every engine (not just resolver-backed); extend the owner-thread destroyEngine guard to fire for any record |
fd49ad1 |
| #5 | Register process beforeExit/exit hooks once per module (never-reset guard) instead of per-singleton |
58c2690 |
| #6 | Real-addon integration tests for the *_engine unknown/destroyed-handle envelope + best-effort run-vs-destroy guard |
cacb41b, 6865803 |
Full Node suite: 885 passed / 59 skipped / 0 failed. Passed a whole-branch review (correctness of the pin across all three run paths, exactly-once release on every early-return, lifecycle-invariant preservation).
Two notes for reviewers
-
Contract change (finding Create LICENSE #1): the owner-thread
destroyEngineguard now keys on "a record exists," not on resolvernapi_refstate — because every engine now carries an env cleanup hook (env-affine state), so a resolver-less engine is also only destroyable from its creating thread. This supersedes the old "resolver-less engines are destroyable from any thread" invariant described in some earlier design docs (docs/superpowers/specs/2026-08-19-worker-teardown-dangling-resolver-ctx-design.mdand the two2026-08-18-*docs).bridge_finalize'snapi_refdeletion stays resolver-gated. -
dwlib C ABI break (finding Set in/out mimetypes as input parameters, env vars #4) is intentional and documented — no compat shims. The branch replaces the
run_script_*_with_resolverexports with the*_enginemulti-engine entrypoints (legacy singletonsrun_script/run_script_callback/run_script_input_output_callbackpreserved) and adds actxparameter toResolveModuleCallback. dwlib is consumed by this repo's own Python/Node bindings in lockstep.
Follow-up not in this round: the runTransform TOCTOU (general review #1) is made memory-safe by the C-side admission pin (worst case is a resolved Unknown engine handle envelope, not a UAF). The additional JS-side recheck/lease the reviewer suggested was deliberately not added — the authoritative pin lives in C. Can add the JS belt-and-suspenders as a separate change if desired.
Round 12 — worker ref-leak & teardown-race hardeningPushed
Concurrency invariants (all re-derived and confirmed in a whole-branch review): exactly-one Tests: 895 passed / 59 skipped / 0 failed. |
Round 13 — per-env init-reference ownership (fixes review #5)Pushed What changed. Replaced the process-global init-reference model — which assumed a strict 1 Mechanism.
Review. Each task passed a spec + quality gate; the whole-branch final review (on the most capable model) independently re-derived the Tests. Full Node suite 897 passed / 59 skipped / 0 failed (added 2 integration tests). The Java layer and Known coverage gap (documented in the test file). The two new |
Round 14 — review #5 remediation (all 7 findings)Addresses every finding from follow-up code review #5 (reviewed head
Invariant preserved throughout: Java side and the legacy singleton entrypoints are untouched this round. 🤖 Generated with Claude Code |
Round 15 — external review #6 remediationAddressed all 8 code findings from the latest follow-up review (
On #5's residual (accepted, documented in-code): if a teardown fails and no later #9 (Python-binding scope) — intentionally not split. The review noted the PR bundles Python-binding modernization alongside the Node multi-engine work. That bundling is intentional for this PR and will not be split out in this round; the Python work is being tracked as part of the same effort. Happy to revisit if a reviewer feels strongly, but flagging it here so the decision is explicit. A whole-branch review of the combined round-15 changes traced the full |
Root-cause fix for the three findings in the sixth PR #157 follow-up review: model the DataWeave instance lifecycle explicitly (uninitialized/ready/ cleaning-up) instead of a single boolean, make C-side stream/transform admission atomic under g_mutex, and validate napi_get_value_int64 at the handle-read sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aaeafb9 to
6fb5603
Compare
Review #7 remediation pushed (
|
| # | Sev | Fix | Commit |
|---|---|---|---|
| #1 | High | Detach the phantom GraalVM thread on a failed (!=0) graal_tear_down_isolate, in both cleanup_thread_fn and teardown_waiter_thread_fn (detach only on the failure branch — never on success/UAF or attach-failure) |
55b2418 |
| #2 | Med | Arm the stranded-teardown retry after a failed init-hook rollback so the next initialize() reclaims the isolate instead of wedging |
ff3b6bb |
| #3 | Med | Model initialize()'s ffi.cleanup() rollback as pending state — observe it (no unhandledRejection) and gate a concurrent initialize() with "cleanup is in progress"; still throws the original error synchronously |
ae35089 |
| #6 | Low | Track native stream rejection by settlement state (startRejected) instead of the startError !== undefined value sentinel, so Promise.reject(undefined) propagates |
eed6ee4 |
| #7 | Low | Fail the worker-lifecycle test when balancing cleanup fails on a passing body (re-throw only when bodySucceeded) |
c258463 |
| #5 | Med | State the final-reference condition in the instance cleanup() docs (resolves after full isolate teardown only when releasing the last instance) |
6462d96 |
| #4 | Med | Root README: await cleanup in the lifecycle example; document the real beforeExit+exit hook pair and signal caveat |
6911de9 |
The final review also tightened the #2 inline comment (c54ff65): that acquire-failure path leaves g_initialized==0, so the retry recovers a transient teardown failure but a persistent graal_tear_down_isolate failure strands the isolate to process exit (pre-existing best-effort degradation) — the prior "adopted by the fast path" wording overstated it.
On finding #8 (Med) — Python-binding modernization scope
Acknowledged, and we agree the Python pytest setup / module extraction / TCK / CI / docs are logically a separate concern from the Node engine-lifecycle work. We're deferring the split to its own follow-up PR rather than doing git surgery on this branch now: the Node lifecycle changes have been through seven review rounds against this exact history, and re-splitting risks disturbing that reviewed state for a packaging reorg. The follow-up PR will carry the Python modernization on its own so its CI/rollout/rollback can be reasoned about independently. This branch's remaining diff remains the Node engine/lifecycle work plus these remediations.
Review #8 remediation — all 7 findings addressed (pushed
|
| # | Finding | Fix | Commit |
|---|---|---|---|
| 1 | High — napi_initialize deadlocks after a repeated stranded-teardown failure (blocks on a uv_cond_wait nobody signals) |
After the retry, detect the unrecoverable stranded state (g_isolate != NULL && !g_initialized && TEARDOWN_NONE) and throw a deterministic error instead of entering the wait loop. g_teardown_needed stays armed and g_ref_count is untouched. |
9fc728f |
| 2 | Med — a synchronous rollback ffi.cleanup() throw strands the instance in "cleaning-up" |
Route the release through a promise boundary so a synchronous throw is normalized to a rejection that still settles the .finally state reset; the caller still synchronously sees the original engine-creation error. |
85d06ae |
| 3 | Med — worker-lifecycle balancing cleanup skips ffi.cleanup() when destroyEngine throws |
Capture the destroy error but always run ffi.cleanup(); surface a balancing failure only when the test body succeeded. |
eccc9d9 (+ 1831ccd) |
| 4 | Med — README teaches invalid terminal-metadata retrieval (generator.return() after for await) |
Document manual next() iteration capturing the terminal { done: true, value: StreamingResult }. |
faf0e78 |
| 5 | Med — README overstates runTransform async input as constant-memory |
Switch large-file examples to a synchronous generator, add the sync-vs-async pre-buffering note, and qualify the Performance claims. | c7799e2 (+ 8378029) |
| 6 | Med — external-modules.md examples omit required cleanup |
Wrap the 5 complete examples in try/finally { await dw.cleanup(); }; label the 2 genuine fragments. |
9eec6b2 |
| 7 | Med — root README overstates cleanup as an unconditional process-wide drain | Qualify the §4 and §9 comments: the drain/teardown happens only when the final shared reference is released. | 8347516 |
Notes
- On finding Create LICENSE #1 and my review Update .travis.yml #7 sign-off: my review Update .travis.yml #7 final review wrongly cleared this as a benign "lingers-to-process-exit" degradation — I conflated the
napi_initializewait path with theisolate_ref_release_n_lockedrelease-path twin (which is benign and adoptable). Finding Create LICENSE #1 was a real hard deadlock (uv_cond_waitholdingg_mutexwith no remaining signaller), and it is now closed. The final review traced the new guard against every state reachable at the wait-loop entry to confirm it fires only in the stranded case and never on a cold start, a healthy re-init, or a legitimate in-flight teardown. - Finding Unable to resolve reference to payload error #2 fix detail: the review's suggested
Promise.resolve().then(() => ffi.cleanup())defersffi.cleanup()into a microtask, which breaks two existing tests that assert synchronous invocation. The shipped fix uses an equivalenttry/catch→Promise.rejectnormalization that preserves the synchronous invocation timing while achieving the same intent. - Two Minor accuracy nits raised by the final review (test double-fault error-masking; a
readFileSyncbounded-memory caveat) are folded into1831ccd/8378029. - Scope unchanged: Node-binding-only; no Python, Java, or legacy-singleton entrypoints touched; handle width stays
long long.
Addresses GUS W-23692110, discovered while implementing Node.js external module support (#154). native-lib's ScriptRuntime is a static singleton with a write-once resolver, so a second DataWeave instance in one Node process silently reuses the first instance's resolver instead of getting its own. Design: turn ScriptRuntime into a handle-addressable registry of per-instance engines (one shared GraalVM isolate, following the pattern native-cli's NativeRuntime already uses), with a per-handle resolver bridge in the Node C addon. Python is out of scope here (tracked as a follow-up) since it already gets isolation via one isolate per instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…egression test Rewires ffi.ts and dataweave.ts to call the new handle-based N-API methods (createEngine/createEngineWithResolver/destroyEngine/ runScriptEngine/runScriptStreamingEngine/runScriptTransformEngine) added in Task 3, removing runWithResolver. Each DataWeave instance now owns its own engineHandle, created on initialize() and destroyed on cleanup(), so multiple instances with different resolvers no longer cross-talk in the same process. Adds independent-engines.test.ts proving two resolver-backed instances resolve only their own modules, that a genuine script error on the new handle-based run() path surfaces as success:false rather than an unhandled throw (runScriptEngine now returns "" instead of throwing on a NULL native result), and that runStreaming/runTransform correctly thread the handle through addon.c's argument-shifted N-API wiring. Deletes the now-obsolete first-resolver-wins regression test and fixture, and rewrites dataweave-resolver.test.ts so each test builds its own minimal resolver map instead of sharing a process-wide "first resolver wins" module map.
…itialize() failure If ffi.initialize() succeeded but engine creation (createEngine/ createEngineWithResolver) then threw, this.initialized stayed false, so cleanup()'s early-return guard meant ffi.cleanup() was never called -- permanently leaking that instance's increment of the native library's ref-counted handle. initialize()'s catch block now releases that ref-count itself (ffi.cleanup()) when ffi.initialize() already succeeded, before wrapping and re-throwing. Adds tests/unit/dataweave-initialize.test.ts, a new unit-lane test (mocked ffi module, no dwlib required) exercising this exact sequencing bug plus the surrounding invariants: no cleanup() call when ffi.initialize() itself fails, no residual state after a failed attempt, and no spurious cleanup() call on the successful path.
…ps (F1, F2) Resolver-backed engine bridges could be freed while a background streaming/ transform uv_thread still dereferenced them via resolve_module_callback (F1), and napi_cleanup deleted thread-affine napi_refs from whatever thread made the last release (F2, undefined behavior across Workers). F1: add in_flight/destroy_pending accounting (under g_mutex). Streaming/transform setup pins the bridge via bridge_begin_op before spawning the worker thread; the completion sentinel releases it via bridge_end_op on the owner thread. destroyEngine unlinks immediately but defers the free (napi_ref delete + struct free) to the last draining op when in_flight > 0. F2: register a per-env cleanup hook (napi_add_env_cleanup_hook) per bridge at creation so each Worker/main env disposes its own napi_ref on its own thread; destroyEngine removes the hook before an early free. napi_cleanup no longer touches g_bridges and only performs the process-global GraalVM isolate teardown once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…k (F3, F4) create_engine/create_engine_with_resolver are GraalVM @CEntryPoints; if Java construction throws, the entrypoint returns the long long default value (0) instead of propagating. Treat any handle <= 0 as invalid: throw an N-API error and unwind the bridge (delete napi_ref, free struct) before it's ever linked into g_bridges or given a cleanup hook, instead of returning/inserting a bogus handle. Also fix a resolver-source buffer leak: if the malloc for the tracking node itself fails, the buffer was previously left untracked and unfreeable. resolver_results_track now reports tracking failure so resolve_module_callback can free the buffer and report "unresolved" instead of leaking it.
… path The handle <= 0 rejection path did manual napi_delete_reference + free(bridge) instead of bridge_finalize, so any resolver-callback buffers already tracked via resolver_results_track (if resolve_module_callback ran during a failed eager module setup before construction was reported as failed) were leaked. bridge_finalize already frees tracked buffers before freeing the struct and is a safe drop-in here since the bridge was never linked into g_bridges or given a cleanup hook at this point.
Node addon.c: fail initialize() with a clear message when dwlib lacks the per-engine symbols (create_engine, create_engine_with_resolver, destroy_engine, run_script_engine, run_script_callback_engine, run_script_input_output_callback_engine) instead of deferring to a confusing per-call error, since every initialize() now creates an engine. CallbackWeaveResourceResolver.resolve(): suppress exception detail by default and only log e.getMessage() when DATAWEAVE_RESOLVER_DEBUG=1, matching the C-side resolve_module_callback policy in addon.c. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…free The default (non-debug) branch of CallbackWeaveResourceResolver.resolve()'s catch block still logged the module path unconditionally, which is dynamic, resolver-controlled content. Drop path too in the default branch so the log line is fully static, matching the C-side resolve_module_callback's actual default behavior in addon.c. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds four resolver-backed integration tests to dataweave-resolver.test.ts that exercise paths untested by prior remediation commits: - a throwing resolveModule() causes run() to fail cleanly (success:false) rather than crash, exercising resolve_module_callback's exception catch/clear/log-gated-by-DATAWEAVE_RESOLVER_DEBUG path. - a resolver-backed instance's initialize -> cleanup -> initialize cycle still resolves a custom module afterwards (fresh engine_bridge_t). - cleanup() raced against an in-flight resolver-backed runStreaming() does not crash -- the regression test for the F1 in-flight-refcount fix, started deterministically by calling gen.next() without awaiting it before calling cleanup(), so the native call is already handed to the libuv worker thread when cleanup() runs on the JS thread. - run() after cleanup() throws DataWeaveError via dataweave.ts's ensureInitialized() guard (the TS-level half of the destroyed/unknown engine handle contract).
Extracts the "Unknown engine handle" JSON literal shared by run_script_engine, run_script_callback_engine, and run_script_input_output_callback_engine into a single package-visible constant (NativeLib.UNKNOWN_ENGINE_HANDLE_JSON), so the exact error contract can be asserted from a plain JVM unit test. The @centrypoint methods themselves can't be exercised directly from a JVM test since their GraalVM word-type parameters (IsolateThread, CCharPointer) only resolve inside a compiled native image. Adds ScriptRuntimeTest#unknownEngineHandleProducesExactErrorJson, which combines that constant assertion with the existing proof that ScriptRuntime.get() returns null for an unregistered handle.
These were internal review artifacts incidentally committed during remediation work (one references a local temp worktree path); they aren't product documentation and shouldn't ship in the repo.
The follow-up PR-157 review found that DataWeave.cleanup() can deadlock the process when called while a runStreaming()/runTransform() operation is still in flight: isolate teardown blocks the JS thread that a mid-delivery worker's threadsafe-function call depends on. This design makes teardown async and wait for active ops to drain via a dedicated waiter thread, instead of blocking inline.
… (review #6 #3/#4) cleanup_thread_fn and teardown_waiter_thread_fn treated a nonzero graal_tear_down_isolate as success, orphaning a live isolate. Set torn_down only on a 0 return. In the async-waiter last-release path, arm g_teardown_needed when teardown did not happen and the isolate is stranded with zero owners, instead of leaving it with no retry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eview #6 #5) Zero-op teardown-failure paths armed g_teardown_needed, but retries fire only at op completion -- with no pending op the isolate was never reclaimed and a naive adoption discarded the pending teardown. Call retry_stranded_teardown_locked() at the top of napi_initialize so a pending teardown is completed (or retried) before adopt/create. Document the residual: with no later op or init, the isolate lingers to process exit (OS reclaims it). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ate (review #6 #6/#7) The cleanup:true worker path swallowed destroyEngine errors, letting a broken destroy pass as a clean lifecycle; fold the error into the posted message with cleanup() still in finally. Wrap the cross-env test in try/finally so a mid-test failure cannot strand a live isolate + held reference for sibling tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on it (review #7 #3) Model the async rollback ffi.cleanup() as pending state (state='cleaning-up' + cleanupPromise) so an un-awaited rejection cannot become an unhandledRejection and a concurrent initialize() is rejected deterministically instead of racing the in-flight release. initialize() stays synchronous. Adapt the pre-existing clean-reinit test to await the rollback settling, per the new gating contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o fast-path adoption (review #7 final) The napi_initialize acquire-failure arm leaves g_initialized==0, so unlike the isolate_ref_release_n_locked twin (g_initialized==1, adopted by the fast path) the retry only recovers a TRANSIENT teardown failure; a persistent graal_tear_down_isolate failure strands the isolate to process exit. The prior comment's 'adopted by the fast path' clause overstated recovery. Comment-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cycle body already failed (review #8 final) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…leSync holds full file) (review #8 final) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
8378029 to
136e914
Compare
…oundary; cover malformed streaming/transform raw-ffi args (review #9 #6) The transform entrypoint silently coerced any non-string inputCharset (objects, numbers, booleans) to NULL, unlike the four non-nullable string args which throw. inputCharset is nullable, so null/undefined still map to "no charset", but any other type is now a caller error that fails closed via TRANSFORM_FAIL. The JS binding only ever sends string|null (dataweave.ts: opts?.charset ?? null), so the production path is unaffected. Also completes the round-7 #2 malformed-arg sweep: adds coverage for streaming inputsJson and transform inputsJson/inputName/inputMimeType/charset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrite 2026-08-07-native-lib-multi-engine-design.md to reflect the converged final state, folding all review-round hardening decisions into the relevant sections (concurrency & lifecycle model, architecture, ABI break, testing posture). Delete the 12 per-round hardening docs; a provenance appendix maps each round to the section it was folded into. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
native-lib's process-wideScriptRuntimesingleton (one engine, write-once resolver, first-caller-wins) with a handle-keyed registry of per-engineScriptRuntimeobjects living in one shared GraalVM isolate — closing W-23692110 for the Node binding.DataWeaveNode instance now owns an independent native engine (its own module resolver and script cache) addressed by an opaque handle, so multiple instances with different resolvers coexist in one process with no cross-talk.run_script,run_script_callback,run_script_input_output_callback) andScriptRuntime.getInstance()are unchanged, so the Python binding is unaffected. Python's own migration to per-instance engines is a separate follow-up.Design
Original design:
docs/superpowers/specs/2026-08-07-native-lib-multi-engine-design.md(commit729ed19). Each subsequent hardening round has its own committed spec underdocs/superpowers/specs/.Post-review hardening
After the initial implementation, the C addon's lifecycle/concurrency and out-of-memory paths were hardened across nine follow-up review rounds — thread-spawn and thread-safe-function failure handling, N-API thread-affinity discipline for resolver bridges, coalesced
cleanup(), a re-init-during-pending-teardown deadlock fix (TEARDOWN_*state machine + live-isolate adoption), atomicg_mutexadmission for all run/stream/transform entrypoints, exhaustivenapi_get_value_*/napi_create_*status checks, OOM-safe setup and worker/callback allocations, and deferral of the engine registry removal until an engine's admitted ops drain (closing the "Unknown engine handle" admission race). Every round was fixed Node-binding-only (Python surface and legacy singletons untouched), documented in a per-round spec underdocs/superpowers/specs/, and re-verified against a green Node suite.Test plan
./gradlew native-lib:test— registry isolation, cross-talk, and built-ins-only engines pass; legacygetInstance()tests unaffected../gradlew native-lib:nativeCompile— newcreate_engine*/*_enginesymbols exported.native-libNode vitest suite — 878 passed / 59 skipped / 0 failed, including theindependent-engines, teardown-deadlock, and admission regressions plus TCK conformance../gradlew native-lib:pythonTest— passing, confirming the legacy Python-facing surface is untouched.🤖 Generated with Claude Code